Review issues #1

hacst opened this Issue Jul 27, 2013 · 7 comments


None yet

2 participants

hacst commented Jul 27, 2013

Unfortunately I can't seem to comment directly in the source-code so I'll just put these here. Haven't tried using the code yet but some things I noticed in the code (line numbers refer to version 0b0b441):

L 53.: Is the date on that author tag correct? If yes the license one isn't
L 67ff.: Visual Studio since 2010 does ship with stdint.h, though I guess there's no harm supporting the old stuff too.
L 79 (and the other defines): Might want to think about using a prefix. It's not that collisions are likely but when they happen....
L 151: For the sake of consistency with the other methods this should probably be unlink()
L 234: Imho this should return the context len via a parameter. Otherwise the chance of missing that this "string" isn't actually zero terminated is pretty big. At the very least the comments should make it clear you mustn't rely on it.
L 259: Should be after L 265
L 427 and others: The difference between set and update is easy to miss. Maybe there are better nouns. I guess setAndCommit is another option.
L 450: ...ByAvatar maybe use something like AvatarAsCamera? Dunno.
L 668: Comment doesn't really say what this does. I guess it won't "commit" by increasing the tick count ;)

L 32.: Date?
L 57.: In other cases you brace single line if bodies
L 70.: You wrote on SF you want C89 compat. In that cases you can't mix commands and variable declarations. All variables go top. You can use "-std=c89 -pedantic -Werror" to make sure gcc/clang cry out if it happens.

Missing license header.

Aren't there env. variables for the vc/sdk paths? Do you really need them? I think what you need should be available in a visual studio shell. Would have the advantage being able to build with multiplt vc version without having to edit the script.

Nothing really major. Rest looked fine to me (bit unused to that style).

zsawyer commented Oct 8, 2013

Thanks for your input, sorry for getting back at this so late.

L 53: author year was wrong
L 67ff: that include if stdint.h is for non-WIN32 and is required for linux to compile correctly
L 79: good point, I will do that later on
L 151: will do
L 234: there is a separate getContextLen() function for that. I am not a fan of using parameters for return purposes. But I will write some clarification in the comment. I will also rearrange the code so that the getContextLen declaration is right underneath getContext for easier spotting.
L 259: yes, that looks ugly xD
L 427 and others: What about changing updateXxx to commitXxx might work too, the fact that parameters are given indicates that the function will use these values, so it implies setting and shows the relation to the "commit()"-function.
L 450: I will probably go with "setVectorsAvatarAndCamera" and remove the "Avatar" as origin notion from comment and parameters (maybe we can find a better name yet).
L 668: Yes, I had to look that up in the code too. uiVersion and uiTick are responsible to tell that the data is up-to-date. If only the ticks were set after the plugin already had timed out the plugin would still not link because the uiVersion was cleared during unlinking. This actually applies to all set methods, basically this is why we need the update-methods, to (re)"lock" the plugin. The comments for setUiTick and updateUiTick definitly need clarification. Right now one might think only uiTick is responsible for the keeping the lock up.

L 32: copy-paste error xD
L 57: will brace it, must have overlooked it when copying it from mumble's wiki.
L 70: Good point. I am going to check if the C89 compatibility will be too much work, I might drop it.

I am not really sure how I should license this as it is mainly adapted from . The default Mumble license might not be compatible/applicable. Will have to look into it.

I found that ugly too but I haven't found those variables yet ("VS100COMNTOOLS" is version specific for 10.0). It might be in PATH but I'd still be missing the SDK directory. My WinXP dev environment didn't have proper env-variables to use :( . I'd have to look into this further.

Again thanks for looking into it. After the changes, do you think we can get this as the base api version we were talking about?

@zsawyer zsawyer was assigned Oct 8, 2013
zsawyer commented Oct 22, 2013

L234: I have reconsidered your suggestion, it is necessary to hand the length and context in a one-step procedure as there might be concurrency issues when an update on the context was made in between the call of getContext and getContextLen.

The concurrency issue is quite obvious for context/context_len but actually applies to all getters. This is a real issue. I am imagining a call chain of these getters where the first will get a result for tick 1 and the rest will get the result for tick 2 - since there was an update in between.

This seems to be a potential flaw in the link-plugin implementation as well. While this may actually be insignificant and possibly negligible it constitutes unexpected behavior. As one would expect all data collected "at the same time" (i.e. the function fetch ) to be related to the same tick/dwcount.

I guess I will have to rethink the getters. As convenient as they are - they are unreliable. Maybe a "uiTick" parameter would specify the validity of the data.

hacst commented Oct 22, 2013

The possibility of races is a known issue for the link protocol. While every link consuming application must be prepared to handle invalid data the effect of the races are rather harmless. The most likely fields to race are the float vectors and if those aren't from the same frame this (usually) won't cause any audible errors. The other fields don't change so much and if you actually happen to race on them you'll have garbage data until the next read (which likely isn't noticeable either). We though about locking schemes but that's not that straight-forward cross-platform.

As to link protocol other feedback: I'll try to get back to you on that as soon as I can but don't be surprised if it takes me a while :(

zsawyer commented Oct 23, 2013

Thanks for the fast reply on the concurrency issues - since it is more or less a blocking task ;) .
I guess I will get the getContextLen function/approach back as to not give the false impression that the API is able to guarantee that there is no race condition for the content and it's length - which of course it isn't.
Also I will add explicit hint that race conditions may occur and that the API user is responsible to check the validity of the data if needed.

@zsawyer zsawyer closed this Oct 23, 2013
@zsawyer zsawyer reopened this Oct 23, 2013
zsawyer commented Oct 24, 2013

Because the race conditions were not documented anywhere I submitted a bug report:

Ok, having slept over this I think it is OK to simply go with the easy way for now. But I still think it is feasible to upgrade the memory structure/management aka the Link-Plugin to resolve those race conditions later on. The API could probably facilitate the cross-platform capabilities where possible (pthread, pthread w32 comes to mind) but also support legacy non-synched structures. This way the API user doesn't have to worry about race conditions but simply uses the API.

While in most games the race-conditions might not have an audible effect I imagine that this is going to be a problem with games that utilize high velocity (flight or space simulators) where movement means a big change in those position vectors. I also remember already having weird audio position effects when in a vehicle in PR:BF2 - that might be related.

What adds to my fear of race conditions is that I imagine that they happen always when the write cycle is faster than the read cycle. I am not sure how likely that is but definitely depends on the implementation of the Link-Plugin using code.

hacst commented Oct 25, 2013

Let's continue that discussion in the bugreport.

@zsawyer zsawyer pushed a commit that referenced this issue Oct 28, 2013
zsawyer ! resolving most parts of issue #1
! fixed date, copyright header
+ introduced global constants' namespace prefix "LINKAPI_"
! comment clarifications
* renamed function doUnlink -> unlinkMumble (because "unlink" is reserved by stdio.h)
* renamed ...VectorsbyAvatar -> ...VectorsAvatarAsCamera functions
* updated test cases to reflect changes
* minor C89 compatibility fixes
zsawyer commented Nov 2, 2013

Changes were made. The concurrency issue is disregarded. I am fairly happy with how hit came out. Still need to compile for OSX and Linux though.

@hacst hacst referenced this issue in mumble-voip/mumble Nov 5, 2013

Link Plugin v1.2.0 concurrency issues #1050

@zsawyer zsawyer closed this Aug 27, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment